Claude finished @rjvelazco's task in 1m 34s —— View job

Rollback Safety Analysis

• Read rollback-unsafe categories reference
• Get full PR diff
• Analyze diff against all unsafe categories
• Apply rollback safety label

Result: ✅ Safe to Roll Back

All 13 changed files are frontend (Angular/TypeScript) and one Language.properties entry. The diff contains no database migrations, no Elasticsearch mapping changes, no Java backend changes, and no REST/GraphQL API contract changes. Here is the full category check:

Summary: The PR is purely UI — Angular component logic and templates for the block editor link popover, image/video TipTap node extensions, CSS layout fixes, and one i18n key addition. ProseMirror node attribute additions (mimeType/width/height/orientation on dotVideo, href/target on dotImage) affect only the Angular/TipTap layer; they are already stored as optional attributes in the block editor JSON and the server-side schema is unchanged. Rolling back to the previous release leaves the Java backend, database, and Elasticsearch index completely untouched.

Label added: AI: Safe To Rollback

🤖 dotBot Review (Bedrock)

Reviewed 13 file(s); 12 candidate(s) → 7 confirmed, 2 uncertain (unverified, kept for review).

⚠️ Coverage capped: 0 file(s) + 2 lower-severity candidate(s) skipped (limits: 40 files, 12 candidates).

Confirmed findings

• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/components/link-popover/link-popover.component.html:78 — Stale displayText value when switching link types
  The displayText form control retains its previous value when switching from text-link to image-link mode. While the UI hides the field via *ngIf, the underlying form control isn't reset. This could lead to accidental inclusion of displayText in image-link submissions via displayText?.value, as the form submission logic doesn't account for the control's visibility state.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/components/link-popover/link-popover.component.ts:453 — Using null instead of undefined for attribute removal in ProseMirror
  In link-popover.component.ts line 453, setting href: null and target: null when removing a link may not properly clear the attributes. ProseMirror typically removes attributes when set to undefined. Using null could leave the attributes present with null values, potentially causing invalid href attributes in rendered HTML. The correct approach is to use undefined to ensure attributes are removed from the node.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/editor.utils.ts:163 — Potential attribute key collision in video data object
  The data object explicitly sets properties like 'video', 'controls', etc., then spreads videoMetaAttrsFromContentlet. If the contentlet-derived attributes contain same keys, they'll overwrite the hardcoded values. Spread order should be reversed to let explicit properties take precedence.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/extensions/nodes/image.extension.ts:189 — Stale node position reference in async handler
  The code calls getPos() after await onClose(), risking an invalid position if document changes occurred while the popover was open. Node positions should be captured synchronously before async operations to ensure validity.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/extensions/nodes/image.extension.ts:189 — Missing position validity check before selection
  The code sets the selection at position 'pos' without validating if it's within the document's bounds. After modifying the transaction with replaceSelectionWith, the original 'pos' might become invalid if the document structure changes. This could lead to runtime errors when attempting to set an invalid TextSelection position. The code should ensure 'pos' is within 0 <= pos <= tr.doc.nodeSize before creating the selection.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/services/dot-upload.service.ts:75 — Missing properties in DotCMSContentlet usage
  The code switched from PublishedAsset to DotCMSContentlet type but there's no evidence DotCMSContentlet contains required 'asset' or 'identifier' properties. Grep shows DotCMSContentlet is defined in dotcms-js/core as having 'identifier' but no 'asset' property, which could lead to runtime errors when accessing 'asset'.
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/services/dot-upload.service.ts:65 — Unsafe type assertion of API response without validation
  The code directly asserts dotAssets[0] as Record<string, DotCMSContentlet> without validation. While the current API might return compatible data, this hard-coded type assertion creates a maintenance risk if the response structure changes. A type guard or runtime validation would make this more robust against API changes.

🔎 Uncertain (could not confirm or disprove — review manually)

• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/components/toolbar/editor-toolbar.store.ts:53 — Missing test coverage for isImageLinked signal behavior tracking image href presence
  Missing test coverage for isImageLinked signal behavior tracking image href presence
• 🟡 Medium core-web/libs/new-block-editor/src/lib/editor/extensions/nodes/video.extension.ts — Added video node attributes lack test coverage for HTML serialization/deserialization
  Added video node attributes lack test coverage for HTML serialization/deserialization

_{us.deepseek.r1-v1:0 · Run: #28463711457 · tokens: in: 96490 · out: 27973 · total: 124463 · calls: 36 · est. ~$0.281}

@claude review this changes, pretty please

Claude finished @rjvelazco's task in 1m 21s —— View job

Code Review

• Gather context and understand the request
• Read all 10 changed files
• Analyze for bugs, correctness, and best practices
• Post review findings

Overall Assessment

This is a clean, well-scoped fix. The core approach is correct: routing the link toolbar button through updateAttributes('dotImage', ...) instead of the text-link insert path is exactly the right TipTap idiom, and the round-trip through href/target on the node's renderHTML/addNodeView is already proven. The layout fixes for the inline-block baseline gap and the Tailwind Typography margin regression are also correct. No data-migration is needed.

Below are the issues I found, from most to least significant.

🔴 Bug: renderHTML writes href="" when href is null

File: extensions/nodes/image.extension.ts:143

const anchorAttrs: Record<string, string> = { href };

When href is null (the default), this emits <a href="null">…</a> in the serialized HTML. The existing guard on line 145 (const inner = href ? ['a', anchorAttrs, img] : img) prevents the anchor being rendered at all, so this doesn't cause a visible bug today. But if renderHTML is ever called on a node with href: null directly (e.g. from a custom serializer or copy/paste), the wrong attribute object gets passed into TipTap's merge. Worth guarding:

const anchorAttrs: Record<string, string> = {};
if (href) anchorAttrs['href'] = href;
if (target) anchorAttrs['target'] = target;
const inner = href ? ['a', anchorAttrs, img] : img;

🟡 Missing test coverage: onRemoveImageLink

The bot flagged this incorrectly as "missing implementation" — onRemoveImageLink is implemented at link-popover.component.ts:433. But there are zero tests for it. The method does the right thing (updateAttributes + close()), but given this is the first time the remove-link path exists, a minimal spec covering: (a) updateAttributes called with { href: null, target: null }, and (b) manager.close() called, would prevent regressions.

🟡 Image-link edit state doesn't pre-populate openInNewTab

File: toolbar.component.ts:421–425

this.popovers.openLink(() => btn.getBoundingClientRect(), {
    isImageLink: true,
    initialValues: attrs['href']
        ? { href: attrs['href'], target: attrs['target'] ?? null }
        : undefined
});

The initialValues includes target, but the form's openInNewTab control is only set from initialValues.target in the pre-populate effect (values.target === '_blank'). Trace the path:

1. Image has target="_blank" → attrs.target = '_blank'
2. initialValues.target = '_blank' is passed correctly
3. The pre-populate effect reads values.target === '_blank' → sets openInNewTab: true ✅

This actually works. But if attrs['target'] is null (no target attribute at all), the ?? null fallback is fine. No bug — just worth a comment clarifying that target flows into openInNewTab via the existing effect.

🟡 ::ng-deep in new CSS (pre-existing pattern, but worth flagging)

File: editor.component.css:244

The PR adds two new ::ng-deep blocks for the linked-image layout fix. ::ng-deep is deprecated in Angular but this file already uses it pervasively (it's the established pattern for styling ProseMirror's DOM from the Angular component). The new rules follow the existing pattern correctly — no change needed, but see Angular deprecation note.

🟡 mousedown + event.preventDefault() on the remove button blocks keyboard access

File: link-popover.component.html:187

(mousedown)="$event.preventDefault(); onRemoveImageLink()"

Using mousedown + preventDefault is the correct pattern for toolbar buttons that must not blur the editor (consistent with every other button in this file). However, the <button> does not have a (click) handler as a fallback, so keyboard users who Tab to this button and press Enter/Space will activate the click event, not mousedown, and nothing will happen. The other destructive buttons in this file (Cancel, Insert) use mousedown on buttons where preventing editor blur is the purpose — but onRemoveImageLink closes the popover and returns focus to the editor anyway, so a (click) handler would actually be fine here:

(click)="onRemoveImageLink()"

Or add both:

(mousedown)="$event.preventDefault()" (click)="onRemoveImageLink()"

🟢 Minor: target attribute not cleared on renderHTML when null

File: extensions/nodes/image.extension.ts:144

const anchorAttrs: Record<string, string> = { href };
if (target) anchorAttrs['target'] = target;

(Fixing the href: null issue above would also naturally express this more clearly.) The current code already conditionally sets target — this part is fine.

🟢 No confirmation before removing the link

The bot flagged this as a concern — I don't share it. A trash icon for a single attribute (not content deletion) is a well-established pattern and the action is undoable via Ctrl+Z. No confirmation dialog needed here.

Summary

The href: null spread is latent (current guard prevents it rendering), but it should be fixed before it causes confusion. Fix this →

Review dispositions

Thanks for the automated reviews. Summary of how each finding was handled:

Addressed

• Overly broad click prevention (editor-chrome-click.ts) — tightened in 814ba6c to gate the link-navigation preventDefault() on the clicked element being the image (event.target.closest('img') within the anchor) rather than any descendant img, so a text click in a mixed anchor still routes to the text-link editor.
• href: null → href="null" in renderHTML (image.extension.ts) — anchorAttrs now built conditionally so a null/absent href never serializes.
• Remove-link button keyboard a11y (link-popover.component.html) — split into (mousedown)="$event.preventDefault()" + (click)="onRemoveImageLink()" so Enter/Space activate it without blurring the editor or double-firing on mouse.

False positives (no change)

• Missing onRemoveImageLink() implementation — the method is implemented at link-popover.component.ts:433; the handler exists and is wired. (The Bedrock/Claude review also corrected this.)
• Image href lacks protocol validation / XSS — image and text links share the same validation. onInsert() returns early on this.form.controls.href.invalid before the image branch, and href carries [Validators.required, linkHrefValidator]. isValidLinkHref allows only http/https/mailto/tel + anchor/relative paths; javascript: parses to protocol === 'javascript:', fails the allowlist, and the form is invalid → nothing is applied. There is no path that sets an image href outside this validated form.

Acknowledged, no change

• ::ng-deep without justification (editor.component.css) — ::ng-deep is the established pattern throughout this file for styling ProseMirror's DOM from the Angular component; the new rules follow it and carry comments explaining the layout rationale.

Note on tests

No spec was added for onRemoveImageLink because libs/new-block-editor/ intentionally has no *.spec.ts files yet (project convention); changes here are verified via nx lint + tsc.

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request